↓Skip to main content
  1. Refs/

Code Review

·3 mins
Table of Contents

Checklist #

Automated checks #

  • Code style (linter)
  • Code formatting (beautifier)
  • Code smells, inefficient algos, security issues (static code analyzer)
  • Basic functionality (unit and integration tests)

Functionality #

  • Happy path / Negative path
  • Edge cases not considered
    • Min / max values
    • Boundary testing
    • Invalid params / special characters
    • Null checks
  • Entry / Exit criteria
  • Look for obvious bugs
    • Off by one
    • Misspelled variables
    • Wrong number of parameters
    • Wrong ordering of parameters

Readability #

  • Idiomatic
  • Should be clear what is happening
  • Descriptive and meaningful names (variables, functions)
  • Complexity / length (lines, functions, classes, modules)
  • Comments
    • Unnecessary comments can be removed
    • Complex logic / regex’s / decisions made should have comments about why (rarely, what)

Maintainability / Extensibility #

  • Structure (folders, files, code)
  • Modular and reusable
    • No code duplication
  • SOLID / DRY / YAGNI
  • Proper logging added
  • Proper exception handling included

Performance #

  • Inefficient algorithms and expensive operations
  • Improper data structures
  • Excessive object allocation
  • Concurrency / race conditions
  • Inefficient string concatenation or logging

Security #

  • No back-doors added
  • Validate inputs, permissions
  • Vulnerability scan for new libraries

Additional hygiene #

  • Branch names and commit messages are correct
    • Can utilize gitmojis for quick / easier parsing of what the commits are doing
  • Any necessary documentation is added / updated
    • README
    • Swagger / OpenAPI specs
    • Any autogen docs

Positive feedback #

  • Call out good practices followed
  • Appreciate how some comments / some problem was addressed
  • Note things that were done well

Go To References #

  • Code Review Developer Guide | Google Eng Practices
  • How to do a code review | Google Eng Practices
    • Generally, approve changes when it definitely improves the overall code health, even if the changes are not perfect
    • Balance making forward progress vs. importance of suggested changes
    • Seek continuous improvement, not perfection
    • Make sure:
      • code is well-designed
      • functionality is good for users (end-users && devs)
      • UI changes are sensible / look good
      • parallel programming is done safely
      • code isn’t more complex than it needs to be
      • things implemented are for now, not for unknown future requirements
      • appropriate unit tests are added
      • tests are well-designed
      • clear names used for everything
      • comments are clear and useful (mostly explain why instead of what)
      • code is appropriately documented
      • code conforms to style guides
    • Make sure to review every line of code, look at the context, ensure code health is being improved, and compliment on good things that were done
    • Include comment severity:
      • Nit: very minor
      • Optional / Consider: good idea, not required
      • FYI: think about this in the future

Videos #

  • How to Do Code Reviews Like a Human - PyGotham 2018
    1. Settle style arguments with a style guide
    2. Let computers do the boring parts
    3. Be generous with code examples
    4. Never say “you”
      Can you rename this variable to something more descriptive like seconds_remaining
      • We: Can we rename this variable to something more…
      • Remove subject: Suggest renaming this variable to something more…
      • Passive voice: This variable should be renamed to something more…
      • What about?: What about renaming this variable to something more…
    5. Frame feedback as requests, not commands
    6. Offer sincere praise
    7. Aim to bring the code up a letter grade or two
      Grade Description
      A Excellent code. I can spot few (if any) minor opportunities to improve clarity
      B Pretty good code. Obviously correct functionally. Could use better naming or a few more tests.
      C Functionally correct, but a bit difficult to understand.
      D Seems functionally correct, but very difficult to read. Requires a major refactor
      F Is either functionally incorrect or so unintelligible that I don’t know if it’s functionally correct
    8. Handle stalemates proactively
↑